Ignore template literals #14173

Trott · 2017-07-11T20:29:03Z

First commit:

tools: ignore template literals for line length

The line length limitation applied by linting is intended to improve
readability. Unfortunately with template literals, practices used to get
around it can inhibit readability. For example:

    return `The "${name}" array must have a length of ${
           length}. Received length ${actual}`;

Configure the linter to ignore line length limitations for template
literals.

Second commit:

lib,test: remove linebreaks from template literals

Improve readability of template literals by removing some creative use
of line breaks.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

tools test lib

The line length limitation applied by linting is intended to improve readability. Unfortunately with template literals, practices used to get around it can inhibit readability. For example: return `The "${name}" array must have a length of ${ length}. Received length ${actual}`; Configure the linter to ignore line length limitations for template literals.

Improve readability of template literals by removing some creative use of line breaks.

AndreasMadsen · 2017-07-11T20:47:29Z

I normally don't care about the linter stuff but this I'm -1 on. Having 80+ lines in annoying for those who don't have a wide screen. And having a single exception for this one case I find very odd.

Surely the solution is to teach/enforce a better approach such as:

const s = `The "${name}" array must have a length of \
${length}. Received length ${actual}`;

or

const s = `The "${name}" array must have a length of` + 
          ` ${length}. Received length ${actual}`;

vsemozhetbyt · 2017-07-11T20:56:34Z

FWIW, we already have exceptions for RegExps and URLs.

Trott · 2017-07-11T21:14:20Z

@AndreasMadsen ~~An alternative might be to use http://eslint.org/docs/rules/template-curly-spacing to get rid of this irksome pattern.~~

On the "annoying for those who don't have a wide screen" thing: Do you fall into that category? I've often suspected that max-len solves a problem that used to be prevalent but is now largely imaginary. It would be more difficult for me to continue to believe that if there was confirmation that you or someone else active on the project works in an environment with an 80-char width limit.

refack

1 Question
1 nit

refack · 2017-07-11T21:29:12Z

test/parallel/test-repl.js

@@ -267,8 +268,8 @@ function error_test() {
      expect: /\bSyntaxError: Invalid or unexpected token/ },
    // do not fail when a String is created with line continuation
    { client: client_unix, send: '\'the\\\nfourth\\\neye\'',
-      expect: `${prompt_multiline}${prompt_multiline}'thefourtheye'\n${
-              prompt_unix}` },
+      expect: `${prompt_multiline}${prompt_multiline}'thefourtheye'\n` +


Why split in this file?

Splitting at \n made sense elsewhere as it made the template literal layout approximate its output, although I can see that maybe it's not needed here.

refack · 2017-07-11T21:30:13Z

test/parallel/test-url-format.js

@@ -253,6 +253,5 @@ for (const u in formatTests) {
  assert.strictEqual(actual, expect,
                     `wonky format(${u}) == ${expect}\nactual:${actual}`);
  assert.strictEqual(actualObj, expect,
-                     `wonky format(${JSON.stringify(formatTests[u])}) == ${
-                     expect}\nactual: ${actualObj}`);
+                     `wonky format(${JSON.stringify(formatTests[u])}) == ${expect}\nactual: ${actualObj}`);


this one could be mitigated by chopping down all args.

refack · 2017-07-11T21:36:46Z

Ref: #8327
Ref: https://drive.google.com/file/d/0Bz0LZMH4OpErSkdSYVRfb2s2QjA/view

refack · 2017-07-11T21:38:42Z

Wrong button

AndreasMadsen · 2017-07-11T22:10:50Z

On the "annoying for those who don't have a wide screen" thing: Do you fall into that category?

This is my normal setup:

I'm sure there are ways I could get 10 chars extra but I don't think that is what we are talking about.

I don't mind 80+ lines when it is clear that no better alternative exists (such as URLs). However, in this case, I strongly believe better alternatives exist.

refack · 2017-07-11T22:22:30Z

@AndreasMadsen could you please repost that in #14176. Personally I'd take 10 more chars 😺

lpinca · 2017-07-12T19:30:36Z

I'm also -1 on this. As @AndreasMadsen said better alternatives exist.

cjihrig · 2017-07-12T20:18:40Z

Also -1

jasnell · 2017-07-14T20:57:24Z

As much as I get annoyed by the 80 char limit, I'm in favor of keeping it. -1 on this.

Trott · 2017-07-15T02:29:58Z

Thanks, everyone! I'm going to close this as (obviously) there is not consensus and I don't intend to argue strenuously for this. I'll instead try to refactor more egregious formatting of template literals into things I find more readable and sensible. New indentation linting may force that anyway.

Trott added 2 commits July 11, 2017 13:23

lib,test: remove linebreaks from template literals

83033fb

Improve readability of template literals by removing some creative use of line breaks.

Trott added lib / src Issues and PRs related to general changes in the lib or src directory. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. labels Jul 11, 2017

nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. tools Issues and PRs related to the tools directory. labels Jul 11, 2017

vsemozhetbyt approved these changes Jul 11, 2017

View reviewed changes

refack suggested changes Jul 11, 2017

View reviewed changes

refack closed this Jul 11, 2017

refack reopened this Jul 11, 2017

gibfahn mentioned this pull request Jul 11, 2017

meta: Are the limits on line length still necessary? #14176

Closed

refack approved these changes Jul 12, 2017

View reviewed changes

Trott closed this Jul 15, 2017

vsemozhetbyt mentioned this pull request Jul 15, 2017

tools: some eslint rules proposals #14253

Closed

Trott deleted the ignore-template-literals branch January 13, 2022 22:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ignore template literals #14173

Ignore template literals #14173

Trott commented Jul 11, 2017

AndreasMadsen commented Jul 11, 2017 •

edited

Loading

vsemozhetbyt commented Jul 11, 2017

Trott commented Jul 11, 2017 •

edited

Loading

refack left a comment

refack Jul 11, 2017

Trott Jul 11, 2017

refack Jul 11, 2017

refack commented Jul 11, 2017

refack commented Jul 11, 2017

AndreasMadsen commented Jul 11, 2017 •

edited

Loading

refack commented Jul 11, 2017

lpinca commented Jul 12, 2017

cjihrig commented Jul 12, 2017

jasnell commented Jul 14, 2017

Trott commented Jul 15, 2017

Ignore template literals #14173

Ignore template literals #14173

Conversation

Trott commented Jul 11, 2017

Checklist

Affected core subsystem(s)

AndreasMadsen commented Jul 11, 2017 • edited Loading

vsemozhetbyt commented Jul 11, 2017

Trott commented Jul 11, 2017 • edited Loading

refack left a comment

Choose a reason for hiding this comment

refack Jul 11, 2017

Choose a reason for hiding this comment

Trott Jul 11, 2017

Choose a reason for hiding this comment

refack Jul 11, 2017

Choose a reason for hiding this comment

refack commented Jul 11, 2017

refack commented Jul 11, 2017

AndreasMadsen commented Jul 11, 2017 • edited Loading

refack commented Jul 11, 2017

lpinca commented Jul 12, 2017

cjihrig commented Jul 12, 2017

jasnell commented Jul 14, 2017

Trott commented Jul 15, 2017

AndreasMadsen commented Jul 11, 2017 •

edited

Loading

Trott commented Jul 11, 2017 •

edited

Loading

AndreasMadsen commented Jul 11, 2017 •

edited

Loading